Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: Import component types from planx-core #2756

Merged
merged 5 commits into from
Feb 14, 2024
Merged

chore: Import component types from planx-core #2756

merged 5 commits into from
Feb 14, 2024

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Feb 5, 2024 •
edited
Loading

What does this PR do?

  • Deletes editor.planx.uk/src/@planx/components/types.ts, replaces with imports from plan-core
  • Find & Replace to swap TYPES.Statement → TYPES.Question
  • Find & Replace to swap TYPES.Response → TYPES.Answer
  • Updates docs

@DafyddLlyr DafyddLlyr force-pushed the dp/import-types-from-planx-core branch from 347f8ad to fd64c10 Compare February 5, 2024 17:13
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 5, 2024 •
edited
Loading

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr force-pushed the dp/import-types-from-planx-core branch 2 times, most recently from 1ebc5c4 to 233ff62 Compare February 6, 2024 09:25
@DafyddLlyr DafyddLlyr marked this pull request as ready for review February 6, 2024 09:27
@DafyddLlyr DafyddLlyr changed the title chore: Import component types from planx-core chore: Import component types from planx-core Feb 6, 2024
@Mike-Heneghan
Copy link
Contributor

Mike-Heneghan commented Feb 6, 2024 •
edited
Loading

nit

I'm not sure if it's part of the scope of this feature although would we want to update the historical records for the AnalyticsSummary analytics_logs to also reflect TYPES.Statement → TYPES.Question && TYPES.Response → TYPES.Answer?

I think this might also require updates to the analytics cards if they're using either of these currently. I don't think they are at the moment but I'd need to double check.

Mike-Heneghan
Mike-Heneghan approved these changes Feb 6, 2024
View reviewed changes
Copy link
Contributor

@Mike-Heneghan Mike-Heneghan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me 👍

It feels much better removing the duplicate file and pointing at the single source of truth.

I ran through a guidance and a submission service on the pizza and everything worked as expected for me.

The only non-blocking implication of this change which I'm not sure if handled yet is the update of Analytics Data which has historical records where I think we might want to update the node types to be consistent with this change as per my comment.

Mike-Heneghan
Mike-Heneghan reviewed Feb 6, 2024
View reviewed changes
hasura.planx.uk/migrations/1707239828082_update_node_type_names/down.sql
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

I don't want to jump the gun but another place node_type is stored as a string is in the metadata when there are back or change events tracked as per: #2654

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thanks for the link to the PR also, super helpful. I've now added migrations to cover this also.

Just as an insurance policy I'd be really grateful if you were able to test these migrations locally juuuust to make sure nothing is amiss. I've tested locally and getting the expected results 👍

@DafyddLlyr DafyddLlyr self-assigned this Feb 7, 2024
@DafyddLlyr DafyddLlyr marked this pull request as draft February 7, 2024 11:57
@DafyddLlyr DafyddLlyr force-pushed the dp/import-types-from-planx-core branch from bfec3be to 56dcac3 Compare February 9, 2024 12:47
@DafyddLlyr DafyddLlyr marked this pull request as ready for review February 9, 2024 14:06
Mike-Heneghan
Mike-Heneghan reviewed Feb 9, 2024
View reviewed changes
hasura.planx.uk/migrations/1707239828082_update_node_type_names/down.sql Outdated
Comment on lines 5 to 6
WHEN node_type 'Question' THEN 'Statement'
WHEN node_type 'Answer' THEN 'Response'
Copy link
Contributor

@Mike-Heneghan Mike-Heneghan Feb 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WHEN node_type 'Question' THEN 'Statement'
WHEN node_type 'Answer' THEN 'Response'
WHEN node_type = 'Question' THEN 'Statement'
WHEN node_type = 'Answer' THEN 'Response'

I was seeing an error with the down migration but I think it was just the = being missing on these lines. I found it surprisingly hard to spot the difference 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good catch, this was me being clumsy using ⌘ + D I think! Fixed here 8b230c9

@DafyddLlyr DafyddLlyr force-pushed the dp/import-types-from-planx-core branch from 8b230c9 to 9c6192e Compare February 13, 2024 15:31
@DafyddLlyr DafyddLlyr force-pushed the dp/import-types-from-planx-core branch from 9c6192e to 10745d2 Compare February 14, 2024 10:47
@DafyddLlyr DafyddLlyr merged commit 7e4e779 into main Feb 14, 2024
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/import-types-from-planx-core branch February 14, 2024 10:58
@DafyddLlyr DafyddLlyr restored the dp/import-types-from-planx-core branch September 12, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mike-Heneghan Mike-Heneghan Mike-Heneghan approved these changes

Assignees

@DafyddLlyr DafyddLlyr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DafyddLlyr @Mike-Heneghan